Detect the used python package and add it when register the python automatically#8
Detect the used python package and add it when register the python automatically#8
Conversation
WalkthroughImplements lazy module registration on first execute/rpc call, adds AST-based per-function import discovery and PackageInfo models, renames plugin API functions, changes session context modules from lists to sets, updates client/session callsites, expands CI workflows, and adds/updates tests and documentation for these behaviors. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Session as D3Session
participant API as Designer API
participant Registry as Module Registry
rect rgba(255, 200, 100, 0.5)
Note over Client,Registry: Old Flow (Explicit Registration)
Client->>Session: with Session(context_modules={module_x})
Session->>API: register_module(module_x)
API->>Registry: Register Module
Client->>Session: execute(payload)
Session->>API: d3_api_execute(payload)
end
rect rgba(100, 200, 255, 0.5)
Note over Client,Registry: New Flow (Lazy Registration)
Client->>Session: execute(payload(moduleName="module_x"))
Session->>Session: Check: "module_x" in registered_modules?
alt Not registered
Session->>API: register_module(module_x)
API->>Registry: Register Module
Session->>Session: Add "module_x" to registered_modules
end
Session->>API: d3_api_execute(payload)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/designer_plugin/d3sdk/client.py (1)
86-89:⚠️ Potential issue | 🟡 MinorUpdate stale docstring references to old function names.
The docstring still references
d3_api_pluginandd3_api_aplugin(line 88), but these have been renamed tod3_api_executeandd3_api_aexecute.📝 Proposed fix
- 5. Sends it to Designer via d3_api_plugin or d3_api_aplugin + 5. Sends it to Designer via d3_api_execute or d3_api_aexecute🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/designer_plugin/d3sdk/client.py` around lines 86 - 89, Update the stale docstring in client.py that still mentions d3_api_plugin and d3_api_aplugin: change those references to the new function names d3_api_execute and d3_api_aexecute and ensure any explanatory text that describes synchronous vs asynchronous calls matches the current behavior of d3_api_execute/d3_api_aexecute; verify the docstring around the PluginPayload/script creation (mentions of "return plugin.{method_name}({args})" and the call flow) uses the new names so readers can find the correct functions..github/workflows/ci.yml (1)
20-41:⚠️ Potential issue | 🟠 MajorWire the matrix Python into uv itself.
The current workflow doesn't properly configure uv to use the matrix Python version. The
uv python installstep installs the version but doesn't guarantee subsequentuv syncanduv runcommands use it. Per official uv documentation, use thepython-versionparameter in theastral-sh/setup-uvaction, which sets theUV_PYTHONenvironment variable for all downstream commands. Without this, all matrix jobs may collapse to the same Python interpreter.Suggested fix
- - name: Install uv + - name: Install uv and set Python ${{ matrix.python-version }} uses: astral-sh/setup-uv@v7 with: enable-cache: true - - - name: Set up Python ${{ matrix.python-version }} - run: uv python install ${{ matrix.python-version }} + python-version: ${{ matrix.python-version }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 20 - 41, The workflow currently installs the matrix Python with a separate "Set up Python" step but doesn't ensure uv uses that interpreter; update the "Install uv" step that uses astral-sh/setup-uv@v7 to include the python-version input set to ${{ matrix.python-version }} (this sets UV_PYTHON for downstream steps) and remove or make the "Set up Python" `uv python install` step redundant; ensure step names ("Install uv", action astral-sh/setup-uv@v7) are the ones updated so subsequent `uv sync` and `uv run` commands use the matrix Python.src/designer_plugin/d3sdk/session.py (3)
32-43:⚠️ Potential issue | 🟡 MinorMinor docstring inconsistency: parameter is
set[str], notlist[str].The docstring at line 38 says "List of module names" but the parameter type is
set[str].📝 Proposed fix
def __init__(self, hostname: str, port: int, context_modules: set[str]) -> None: """Initialize base session with connection details and module context. Args: hostname: The hostname of the Designer instance. port: The port number of the Designer instance. - context_modules: List of module names to register when entering session context. + context_modules: Set of module names to register when entering session context. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/designer_plugin/d3sdk/session.py` around lines 32 - 43, Docstring for __init__ incorrectly describes context_modules as a "List of module names" while the parameter type is set[str]; update the docstring for the __init__ method (the constructor that assigns self.hostname, self.port, self.context_modules, self.registered_modules) to refer to "set of module names" (or "collection/set of module names") so the parameter description matches the type annotation.
57-66:⚠️ Potential issue | 🟡 MinorDocstring inconsistency: type is
set[str], notlist[str].Line 64 says "Optional list of module names" but the parameter type is
set[str] | None.📝 Proposed fix
"""Initialize synchronous Designer session. Args: hostname: The hostname of the Designer instance. port: The port number of the Designer instance. - context_modules: Optional list of module names to register when entering session context. + context_modules: Optional set of module names to register when entering session context. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/designer_plugin/d3sdk/session.py` around lines 57 - 66, The docstring for the __init__ method incorrectly calls context_modules an "Optional list of module names" while the parameter type is set[str] | None; update the docstring in the __init__ (the constructor that accepts context_modules) to describe it as an "optional set of module names" (or "optional collection/set of module names") so the text matches the type annotation for context_modules and adjust any wording around registration when entering session context accordingly.
194-203:⚠️ Potential issue | 🟡 MinorDocstring inconsistency: type is
set[str], notlist[str].Line 201 says "Optional list of module names" but the parameter type is
set[str] | None.📝 Proposed fix
"""Initialize asynchronous Designer session. Args: hostname: The hostname of the Designer instance. port: The port number of the Designer instance. - context_modules: Optional list of module names to register when entering session context. + context_modules: Optional set of module names to register when entering session context. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/designer_plugin/d3sdk/session.py` around lines 194 - 203, The docstring for the session initializer incorrectly refers to "Optional list of module names" while the parameter context_modules is typed as set[str] | None; update the docstring in the __init__ method (the session initializer) to reference a set (e.g., "Optional set of module names to register when entering session context") or otherwise align the type and description (change the type to list[str] if you intend a list) so the parameter description matches the declared type context_modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 86-91: The README currently shows an unconditional import of
designer_plugin.pystub which makes the stub a runtime dependency and causes
automatic import detection (see ast_utils.py) to include it in remote payloads;
change the examples to use typing.TYPE_CHECKING and wrap the import in an if
TYPE_CHECKING: block (i.e., reference TYPE_CHECKING and import
designer_plugin.pystub only inside that conditional) so the stubs remain
available for IDE/type hints but are excluded from runtime/import detection used
for remote execution.
In `@src/designer_plugin/d3sdk/ast_utils.py`:
- Around line 56-66: The mypy error is caused by assigning different AST node
types to the same variable `node` in `to_import_statement`; update `node` with
an explicit union type annotation (e.g., ast.Import | ast.ImportFrom) at its
declaration inside the `to_import_statement` method so both `ast.ImportFrom` and
`ast.Import` assignments are type-compatible for `ast.unparse`.
- Around line 446-453: The type-narrowing issue comes from assigning an
ast.Attribute to `root` then reassigning it to `root.value` (an ast.expr),
confusing mypy; update the declaration so `root` is typed as `ast.expr` (e.g.,
replace `root = node` with an explicit annotation `root: ast.expr = node`) so
the while loop `while isinstance(root, ast.Attribute): root = root.value` and
the subsequent `isinstance(root, ast.Name)` check are type-correct for mypy in
the function that processes `ast.Attribute` nodes.
In `@src/designer_plugin/d3sdk/function.py`:
- Around line 271-281: The current replacement of D3Function only updates the
in-process registry and doesn’t force re-registration in existing
D3Session/D3AsyncSession instances that cache modules in registered_modules;
modify the design to track a module version or dirty flag on D3Function (e.g.,
bump a version or set is_dirty when replaced) and store that version in each
session’s registered_modules map, then change D3Session.register_module (or the
callers execute()/rpc()) to compare the local D3Function module version against
the session’s cached version and force re-registration when they differ; update
D3Function._available_d3functions management to increment the module
version/mark dirty on replacement and add a regression test that opens a
session, calls execute(), redefines the function locally, and verifies
subsequent execute()/rpc() triggers re-registration and runs the new code.
- Around line 121-130: find_imports_for_function currently inspects only names
referenced in the function body, missing imports used in defaults/kw-defaults at
definition time; update find_imports_for_function (and any callers that build
packages for FunctionInfo) to traverse the entire AST FunctionDef node including
args.defaults, args.kw_defaults, and decorator_list to collect imports required
at definition time, then include those packages in the
FunctionInfo(packages=...) payload; add a regression test that defines a
function using an import in a default (e.g., def f(now=datetime.datetime.now()):
...) and asserts registration succeeds and the discovered packages include the
datetime import.
- Around line 266-281: When replacing a D3Function you currently discard the old
function but never remove its package imports, so
_available_packages[module_name] grows stale; fix by rebuilding
D3Function._available_packages[module_name] from the current set of registered
functions for that module: after you discard the old function and add the new
one to D3Function._available_d3functions[module_name] (using the existing
module_name and self._function_info.packages symbols), recompute
D3Function._available_packages[module_name] as the union of
pkg.to_import_statement() for every pkg in every f._function_info.packages for
each f in D3Function._available_d3functions[module_name]; assign that rebuilt
set back to D3Function._available_packages[module_name] so removed imports are
no longer present.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 20-41: The workflow currently installs the matrix Python with a
separate "Set up Python" step but doesn't ensure uv uses that interpreter;
update the "Install uv" step that uses astral-sh/setup-uv@v7 to include the
python-version input set to ${{ matrix.python-version }} (this sets UV_PYTHON
for downstream steps) and remove or make the "Set up Python" `uv python install`
step redundant; ensure step names ("Install uv", action astral-sh/setup-uv@v7)
are the ones updated so subsequent `uv sync` and `uv run` commands use the
matrix Python.
In `@src/designer_plugin/d3sdk/client.py`:
- Around line 86-89: Update the stale docstring in client.py that still mentions
d3_api_plugin and d3_api_aplugin: change those references to the new function
names d3_api_execute and d3_api_aexecute and ensure any explanatory text that
describes synchronous vs asynchronous calls matches the current behavior of
d3_api_execute/d3_api_aexecute; verify the docstring around the
PluginPayload/script creation (mentions of "return plugin.{method_name}({args})"
and the call flow) uses the new names so readers can find the correct functions.
In `@src/designer_plugin/d3sdk/session.py`:
- Around line 32-43: Docstring for __init__ incorrectly describes
context_modules as a "List of module names" while the parameter type is
set[str]; update the docstring for the __init__ method (the constructor that
assigns self.hostname, self.port, self.context_modules, self.registered_modules)
to refer to "set of module names" (or "collection/set of module names") so the
parameter description matches the type annotation.
- Around line 57-66: The docstring for the __init__ method incorrectly calls
context_modules an "Optional list of module names" while the parameter type is
set[str] | None; update the docstring in the __init__ (the constructor that
accepts context_modules) to describe it as an "optional set of module names" (or
"optional collection/set of module names") so the text matches the type
annotation for context_modules and adjust any wording around registration when
entering session context accordingly.
- Around line 194-203: The docstring for the session initializer incorrectly
refers to "Optional list of module names" while the parameter context_modules is
typed as set[str] | None; update the docstring in the __init__ method (the
session initializer) to reference a set (e.g., "Optional set of module names to
register when entering session context") or otherwise align the type and
description (change the type to list[str] if you intend a list) so the parameter
description matches the declared type context_modules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2712682c-bc61-44e0-941c-dd73435d3adb
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/ci.yml.github/workflows/test-publish.ymlCHANGELOG.mdCONTRIBUTING.mdREADME.mdpyproject.tomlsrc/designer_plugin/api.pysrc/designer_plugin/d3sdk/__init__.pysrc/designer_plugin/d3sdk/ast_utils.pysrc/designer_plugin/d3sdk/builtin_modules.pysrc/designer_plugin/d3sdk/client.pysrc/designer_plugin/d3sdk/function.pysrc/designer_plugin/d3sdk/session.pytests/test_ast_utils.pytests/test_client.pytests/test_core.pytests/test_session.pytests/test_supported_modules.py
9f76c63 to
6e33aa4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/designer_plugin/d3sdk/ast_utils.py`:
- Around line 505-523: The current import collection only iterates
module_tree.body and therefore misses imports inside the function body and
imports nested inside module-level control blocks (If/Try) or inside the
function node itself; update the import-discovery to walk the entire AST subtree
for relevant Import and ImportFrom nodes (e.g., use ast.walk on module_tree and
also inspect func_node for local imports), then filter those imports by
used_names from _collect_used_names and populate packages (PackageInfo)
accordingly; ensure you still skip TYPE_CHECKING blocks via
_is_type_checking_block and preserve existing behavior for async vs sync
function nodes.
- Around line 524-531: The import handling currently treats the full dotted
module name (alias.name) as the bound name, which is wrong for statements like
"import sklearn.linear_model" where Python binds only "sklearn"; update the
binding logic in the ast.Import branch (inside the function processing
node/alias) so that effective_name = alias.asname if alias.asname else
alias.name.split(".")[0] (i.e., take the top-level package when there is no
alias), then use that effective_name when checking membership in used_names and
for downstream dependency detection; keep the existing _is_builtin_package check
and other logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0282693f-e5a8-4d1b-b4cf-990554e0be5a
📒 Files selected for processing (1)
src/designer_plugin/d3sdk/ast_utils.py
0768cd1 to
ff7d6e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/designer_plugin/d3sdk/ast_utils.py`:
- Around line 460-462: The function _is_type_checking_block currently only
detects bare `if TYPE_CHECKING:`; update it to also accept a qualified form like
`if typing.TYPE_CHECKING:` by checking for ast.Attribute where node.test is an
Attribute with .attr == "TYPE_CHECKING" and the Attribute.value is a Name with
id "typing" (in addition to the existing Name check for "TYPE_CHECKING"). Ensure
the function still returns True for the original bare Name case and False
otherwise.
In `@tests/test_core.py`:
- Around line 529-536: The test incorrectly expects function-local imports to be
detected by find_imports_for_function; update the test so import os is at
module-level (not inside func_b) and ensure func_b references the module-level
symbol (e.g., call os.getcwd()), then assert that "import os" appears in
D3Function._available_packages[module]; keep func_b and the D3Function
assertions unchanged otherwise and reference func_b, find_imports_for_function,
and D3Function._available_packages when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a57ba3b-96e9-4752-82ef-9158b5915e0e
📒 Files selected for processing (13)
CHANGELOG.mdCONTRIBUTING.mdREADME.mdpyproject.tomlsrc/designer_plugin/d3sdk/__init__.pysrc/designer_plugin/d3sdk/ast_utils.pysrc/designer_plugin/d3sdk/builtin_modules.pysrc/designer_plugin/d3sdk/client.pysrc/designer_plugin/d3sdk/function.pysrc/designer_plugin/d3sdk/session.pytests/test_ast_utils.pytests/test_core.pytests/test_supported_modules.py
✅ Files skipped from review due to trivial changes (5)
- src/designer_plugin/d3sdk/client.py
- src/designer_plugin/d3sdk/session.py
- CONTRIBUTING.md
- src/designer_plugin/d3sdk/builtin_modules.py
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/designer_plugin/d3sdk/init.py
- pyproject.toml
- tests/test_supported_modules.py
- CHANGELOG.md
ff7d6e5 to
7fe02a8
Compare
7fe02a8 to
8327bc3
Compare
| This function walks up the call stack to find the module where it was called from, | ||
| then parses that module's source code to extract all import statements that are | ||
| compatible with Python 2.7 and safe to send to Designer. | ||
| @functools.cache |
There was a problem hiding this comment.
This can potentially grow cache infinitely, but I think it's not likely case. I can add cache limit with lru_cache if preferred.
There was a problem hiding this comment.
I'd recommend it - the module reload will make a new one every time and this will hold references to potentially large modules and could conceivably cause issues with import or other long-held references
| "tarfile", | ||
| "telnetlib", | ||
| "test", | ||
| "threading", |
There was a problem hiding this comment.
not allowed threading
| "smtpd", | ||
| "smtplib", | ||
| "ssl", | ||
| "sys", |
| D3Function._available_d3functions[module_name].discard(self) | ||
| D3Function._available_d3functions[module_name].add(self) | ||
|
|
||
| if is_replacement: |
There was a problem hiding this comment.
this is for jupyter notebook support
Summary
@d3functionnow automatically detects file-level imports used by the decorated function and includes them in the registered module. This replaces the manualadd_packages_in_current_file()helper, which required an explicit call and included all imports from the file regardless of usage.Old way
New way
How it works
@d3functiondecorates a function,find_imports_for_function()parses the module's AST to collect all top-level import statements.SUPPORTED_MODULES) are included in the registration payload.Key changes
find_imports_for_function(func)— new AST-based import extraction, replacing the stack-frame-basedfind_packages_in_current_file()PackageInfo/ImportAliasmodels — structured representation of imports withto_import_statement()rendering viaast.unparseSUPPORTED_MODULESallowlist — only Designer-supported stdlib modules are forwarded; unsupported or third-party imports are excludedTYPE_CHECKINGexclusion — imports insideif TYPE_CHECKING:orif typing.TYPE_CHECKING:blocks are never includedRemoved
add_packages_in_current_file()— no longer neededfind_packages_in_current_file()— replaced byfind_imports_for_function()Other
pytest -m integration) for validating supported/unsupported modules against a live Designer instanceTest plan
PackageInfo.to_import_statement()(all import styles)find_imports_for_function()(used/unused imports, submodules, dotted imports, no-source fallback)D3Function(accumulation, stale eviction, payload inclusion)pytest -m integration)🤖 Generated with Claude Code